Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 13, 2025

gl.getAttribLocation() was returning 0 for built-in attributes (gl_VertexID, gl_InstanceID, gl_Position, gl_PointSize) instead of the spec-mandated -1, causing Khronos conformance test failures.

Changes

  • src/client/graphics/webgl_context.cpp
    • Added early return check for built-in attribute names in WebGLContext::getAttribLocation()
    • Returns nullopt for built-in attributes (converted to -1 by script bindings layer)
    • Added <set> include
// WebGL 2.0 spec §5.18: Built-in attributes always return -1
static const std::set<string> kBuiltInAttribs = {
  "gl_VertexID", "gl_InstanceID", "gl_Position", "gl_PointSize"};
if (kBuiltInAttribs.count(name))
{
  return nullopt;  // Returns nullopt which script bindings converts to -1
}

Architecture

Implementation is in the graphics layer (client/graphics/webgl_context.cpp) rather than script bindings layer, providing better separation of concerns and centralizing WebGL spec compliance logic at the appropriate architectural level.

Spec Reference

WebGL 2.0 §5.18: "Applications may query the location of a built-in attribute by passing its name to getAttribLocation, but this will always return -1."

Original prompt

This section details on the original issue you should resolve

<issue_title>WebGL2: gl.getAttribLocation for built-in attribute 'gl_VertexID' returns incorrect value (should be -1, got 0)</issue_title>
<issue_description>## Bug Description
Running Khronos/WebGL2 conformance test (active-built-in-attribs.html) in JSAR Runtime:

shouldBe('activeInfo.name', '"gl_VertexID"');
attribLoc = gl.getAttribLocation(prog, 'gl_VertexID');
wtu.glErrorShouldBe(gl, gl.NO_ERROR, "Should be able to request the location of a built-in.");
shouldBe('attribLoc', '-1');

Test fails: attribLoc should be -1. Was 0.

Spec Reference

Root Cause

  • JSAR currently returns 0 for built-in attribute names instead of -1, violating spec compliance.
  • Correct behavior: gl.getAttribLocation(program, 'gl_VertexID') MUST return -1, never a valid location.

Acceptance Criteria

  • getAttribLocation on any built-in attribute ('gl_VertexID', 'gl_InstanceID', 'gl_Position', 'gl_PointSize') MUST always return -1
  • The referenced conformance test passes

Implementation Guidance (C++)

  • Fix the attribute location query in WebGLRenderingContext/WebGL2RenderingContext
  • Check if the attribute name matches any of the built-in names, and always return -1
int WebGLRenderingContext::getAttribLocation(WebGLProgram* program, const std::string& name) {
    static const std::set<std::string> kBuiltInAttribs = {
        "gl_VertexID", "gl_InstanceID", "gl_Position", "gl_PointSize"
    };
    if (kBuiltInAttribs.count(name)) {
        return -1;
    }
    // ... normal lookup logic
}
  • Update error handling and unit tests for built-in attribute query

Impact

  • Strict compliance with WebGL2 specification
  • Enables all Khronos/WebGL2 tests for built-in attrib queries to pass

Relevant test HTML:

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

- Add check for built-in attribute names (gl_VertexID, gl_InstanceID, gl_Position, gl_PointSize)
- Return -1 immediately for built-in attributes per WebGL 2.0 spec §5.18
- Add <set> include for std::set container
- Complies with WebGL 2.0 specification requirement

Co-authored-by: EndlessJour9527 <[email protected]>
Copilot AI changed the title [WIP] Fix gl.getAttribLocation for gl_VertexID returning incorrect value Fix getAttribLocation to return -1 for built-in attributes per WebGL 2.0 spec Nov 13, 2025
Copilot finished work on behalf of EndlessJour9527 November 13, 2025 08:02
Comment on lines 2642 to 2650
// WebGL 2.0 spec §5.18: Built-in attributes always return -1
static const std::set<string> kBuiltInAttribs = {
"gl_VertexID", "gl_InstanceID", "gl_Position", "gl_PointSize"};
if (kBuiltInAttribs.count(name))
{
info.GetReturnValue().Set(Number::New(isolate, -1));
return;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please impl in client/graphics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Moved the implementation to client/graphics/webgl_context.cpp in commit c135954. The built-in attribute check now happens at the graphics layer in WebGLContext::getAttribLocation(), which returns nullopt for built-in attributes. The script bindings layer correctly converts this to -1.

- Moved implementation from script bindings to WebGLContext::getAttribLocation
- Returns nullopt for built-in attributes (gl_VertexID, gl_InstanceID, gl_Position, gl_PointSize)
- Script bindings layer now correctly converts nullopt to -1
- Better architecture: centralizes logic at the graphics layer

Addresses review feedback from @EndlessJour9527

Co-authored-by: EndlessJour9527 <[email protected]>
Copilot finished work on behalf of EndlessJour9527 November 14, 2025 09:49
@EndlessJour9527 EndlessJour9527 marked this pull request as ready for review November 17, 2025 05:56
Copilot finished reviewing on behalf of EndlessJour9527 November 17, 2025 05:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes WebGL 2.0 spec compliance for getAttribLocation() to correctly return -1 for built-in attributes (gl_VertexID, gl_InstanceID, gl_Position, gl_PointSize) instead of incorrectly returning 0. This resolves Khronos WebGL 2.0 conformance test failures.

Key Changes

  • Added early return check in WebGLContext::getAttribLocation() to detect built-in attribute names and return nullopt (which the script bindings layer converts to -1)
  • Applied consistent C++ code formatting across multiple WebGL script binding files (removing extra spaces in closing namespace comments)
  • Updated package-lock.json registry URLs from npmmirror.com to npmjs.org

Reviewed Changes

Copilot reviewed 1 out of 18 changed files in this pull request and generated no comments.

File Description
src/client/graphics/webgl_context.cpp Added built-in attribute check with early return in getAttribLocation(), plus <set> include
src/client/script_bindings/webgl/*.{hpp,cpp} Formatting fix: standardized namespace closing comment spacing
package-lock.json Changed registry URLs from npmmirror.com to npmjs.org for all packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebGL2: gl.getAttribLocation for built-in attribute 'gl_VertexID' returns incorrect value (should be -1, got 0)

2 participants